fix(core): memory leaks from connected callback states#129
Open
coryrylan wants to merge 2 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors form and controller listener lifecycles to return cleanup handles, broadens control reset logic, adds DOM observer/property cleanup and EyeDropper logging guards, replaces several thrown errors with fallback behaviors, and adds tests for reset, reconnect, accessibility, SSR, and fallback paths. ChangesForm control state and reset handling
Controller event listener cleanup patterns
DOM utility cleanup support and environment safety
Graceful error handling and fallback behavior
PagePanel rendering tests and dependency updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/core/src/internal/controllers/type-native-popover.controller.ts (1)
165-180:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard async legacy-trigger setup after RAF to prevent stale observers.
#setupLegacyTriggers()can create aMutationObserverafter disconnect (Line 166 await gap), which bypasseshostDisconnected()cleanup and can reintroduce duplicate observer behavior on reconnect.Suggested fix
async `#setupLegacyTriggers`() { await new Promise(r => requestAnimationFrame(r)); + if (!this.host.isConnected) { + return; + } // setup hidden updates for legacy triggers this.#observers.push( getAttributeListChanges(this.host, ['hidden'], () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/internal/controllers/type-native-popover.controller.ts` around lines 165 - 180, The async gap in `#setupLegacyTriggers` can create and register a MutationObserver after the host has been disconnected, bypassing hostDisconnected cleanup and causing duplicate observers on reconnect; fix this by checking the host connectivity immediately after the await (e.g., if (!this.host.isConnected) return) before calling getAttributeListChanges and pushing into this.#observers, ensuring you only create/register the observer while the host is still connected; reference `#setupLegacyTriggers`, getAttributeListChanges, this.host.isConnected, and this.#observers to locate where to add the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-35: The test currently calls removeFixture(fixture) only on the
success path, which can leak DOM if elementIsStable or runAxe throws; wrap the
async test body so that after creating fixture with createFixture(...) you
always call removeFixture(fixture) in a finally block (or ensure teardown via
afterEach) so the fixture is removed regardless of failures, adjusting
references to createFixture, elementIsStable, runAxe, and removeFixture in the
test named 'should pass axe check'.
---
Outside diff comments:
In `@projects/core/src/internal/controllers/type-native-popover.controller.ts`:
- Around line 165-180: The async gap in `#setupLegacyTriggers` can create and
register a MutationObserver after the host has been disconnected, bypassing
hostDisconnected cleanup and causing duplicate observers on reconnect; fix this
by checking the host connectivity immediately after the await (e.g., if
(!this.host.isConnected) return) before calling getAttributeListChanges and
pushing into this.#observers, ensuring you only create/register the observer
while the host is still connected; reference `#setupLegacyTriggers`,
getAttributeListChanges, this.host.isConnected, and this.#observers to locate
where to add the guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5192ca9b-8cbf-4a28-b047-c6a42d58991e
📒 Files selected for processing (26)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.ts
Comment on lines
+13
to
+35
| it('should pass axe check', async () => { | ||
| const fixture = await createFixture(html` | ||
| <nve-page> | ||
| <nve-page-panel id="panel"> | ||
| <nve-page-panel-header>panel heading</nve-page-panel-header> | ||
| <nve-icon-button | ||
| slot="actions" | ||
| commandfor="panel" | ||
| command="--close" | ||
| icon-name="cancel" | ||
| aria-label="close panel"> | ||
| </nve-icon-button> | ||
| <nve-page-panel-content>panel content</nve-page-panel-content> | ||
| <nve-page-panel-footer>panel footer</nve-page-panel-footer> | ||
| </nve-page-panel> | ||
| </nve-page> | ||
| `); | ||
|
|
||
| await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); | ||
| const results = await runAxe([PagePanel.metadata.tag]); | ||
| expect(results.violations.length).toBe(0); | ||
| removeFixture(fixture); | ||
| }); |
There was a problem hiding this comment.
Ensure fixture teardown always runs.
removeFixture(fixture) is only reached on the success path. If elementIsStable or runAxe throws, the fixture remains in document.body, which can leak DOM across tests and cause follow-on flakiness.
Suggested fix
describe(PagePanel.metadata.tag, () => {
it('should pass axe check', async () => {
const fixture = await createFixture(html`
...
`);
- await elementIsStable(fixture.querySelector(PagePanel.metadata.tag));
- const results = await runAxe([PagePanel.metadata.tag]);
- expect(results.violations.length).toBe(0);
- removeFixture(fixture);
+ try {
+ await elementIsStable(fixture.querySelector(PagePanel.metadata.tag));
+ const results = await runAxe([PagePanel.metadata.tag]);
+ expect(results.violations.length).toBe(0);
+ } finally {
+ removeFixture(fixture);
+ }
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should pass axe check', async () => { | |
| const fixture = await createFixture(html` | |
| <nve-page> | |
| <nve-page-panel id="panel"> | |
| <nve-page-panel-header>panel heading</nve-page-panel-header> | |
| <nve-icon-button | |
| slot="actions" | |
| commandfor="panel" | |
| command="--close" | |
| icon-name="cancel" | |
| aria-label="close panel"> | |
| </nve-icon-button> | |
| <nve-page-panel-content>panel content</nve-page-panel-content> | |
| <nve-page-panel-footer>panel footer</nve-page-panel-footer> | |
| </nve-page-panel> | |
| </nve-page> | |
| `); | |
| await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); | |
| const results = await runAxe([PagePanel.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| removeFixture(fixture); | |
| }); | |
| it('should pass axe check', async () => { | |
| const fixture = await createFixture(html` | |
| <nve-page> | |
| <nve-page-panel id="panel"> | |
| <nve-page-panel-header>panel heading</nve-page-panel-header> | |
| <nve-icon-button | |
| slot="actions" | |
| commandfor="panel" | |
| command="--close" | |
| icon-name="cancel" | |
| aria-label="close panel"> | |
| </nve-icon-button> | |
| <nve-page-panel-content>panel content</nve-page-panel-content> | |
| <nve-page-panel-footer>panel footer</nve-page-panel-footer> | |
| </nve-page-panel> | |
| </nve-page> | |
| `); | |
| try { | |
| await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); | |
| const results = await runAxe([PagePanel.metadata.tag]); | |
| expect(results.violations.length).toBe(0); | |
| } finally { | |
| removeFixture(fixture); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 -
35, The test currently calls removeFixture(fixture) only on the success path,
which can leak DOM if elementIsStable or runAxe throws; wrap the async test body
so that after creating fixture with createFixture(...) you always call
removeFixture(fixture) in a finally block (or ensure teardown via afterEach) so
the fixture is removed regardless of failures, adjusting references to
createFixture, elementIsStable, runAxe, and removeFixture in the test named
'should pass axe check'.
coryrylan
force-pushed
the
topic-cleanup-core
branch
from
97a3f25 to
84daad7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
31-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuarantee fixture teardown on failure paths.
removeFixture(fixture)only runs on success. If Line 31 or Line 32 throws, the fixture leaks into later tests.Suggested fix
it('should pass axe check', async () => { const fixture = await createFixture(html` ... `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 31 - 34, The test currently calls removeFixture(fixture) only on the success path which leaks DOM when elementIsStable(fixture.querySelector(PagePanel.metadata.tag)) or runAxe([PagePanel.metadata.tag]) throws; wrap the test actions in a try/finally (or equivalent test-framework teardown) so removeFixture(fixture) is executed in the finally block to guarantee cleanup; locate the test containing elementIsStable, runAxe, PagePanel.metadata.tag and fixture and move the removeFixture(fixture) call into the finally section (or add an afterEach that calls removeFixture using the same fixture variable) to ensure teardown on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/format-relative-time/format-relative-time.test.ts`:
- Around line 412-424: The Date.now spy named now in the test 'should fall back
to raw string for relative values outside Intl range' can leak if an assertion
throws; wrap the setup and assertions so the spy is always restored (e.g., move
now.mockRestore() into a finally block or use a try/finally around the test body
or an afterEach that restores the spy), ensuring the spy created by
vi.spyOn(Date, 'now') is always cleaned up even if element.requestUpdate() or
the expect throws; reference the test name and the now spy to locate and update
the code.
In `@projects/core/src/forms/control/control.css`:
- Around line 189-190: The declaration-empty-line-before lint violation is
caused by missing a blank line before the `width` declaration; edit the block
containing `--control-height: auto;` and `width: fit-content;` (the
`--control-height` custom property and the `width` property) and insert a single
empty line before `width: fit-content;` so the `width` declaration is separated
from the preceding custom property, satisfying the stylelint
`declaration-empty-line-before` rule.
In `@projects/core/src/select/select.test.ts`:
- Around line 283-286: The test sets option.selected = true and immediately
asserts requestUpdate wasn't called, but MutationObserver callbacks are async;
update the test to wait for observer delivery before the negative assertion by
using the project's test helper pattern (e.g., call elementIsStable(element) or
await the fixture/frame microtask after setting option.selected) so any delayed
requestUpdate calls run first; locate the assertion around the spy on
element.requestUpdate in select.test.ts and insert the await/elementIsStable
wait before the expect(requestUpdate).not.toHaveBeenCalled() check.
---
Duplicate comments:
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 31-34: The test currently calls removeFixture(fixture) only on the
success path which leaks DOM when
elementIsStable(fixture.querySelector(PagePanel.metadata.tag)) or
runAxe([PagePanel.metadata.tag]) throws; wrap the test actions in a try/finally
(or equivalent test-framework teardown) so removeFixture(fixture) is executed in
the finally block to guarantee cleanup; locate the test containing
elementIsStable, runAxe, PagePanel.metadata.tag and fixture and move the
removeFixture(fixture) call into the finally section (or add an afterEach that
calls removeFixture using the same fixture variable) to ensure teardown on
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cb4672ee-95ad-43e9-821e-e07486e03d60
📒 Files selected for processing (26)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.ts
Comment on lines
+412
to
+424
| it('should fall back to raw string for relative values outside Intl range', async () => { | ||
| vi.useRealTimers(); | ||
| const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); | ||
|
|
||
| element.date = '2023-07-27T12:00:00.000Z'; | ||
| element.requestUpdate(); | ||
| await elementIsStable(element); | ||
|
|
||
| const time = element.shadowRoot!.querySelector('time'); | ||
| expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); | ||
|
|
||
| now.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Ensure Date.now spy is always restored, even on assertion failure.
now.mockRestore() currently runs only on the happy path. If the test throws earlier, the global spy can leak into later tests.
Proposed fix
it('should fall back to raw string for relative values outside Intl range', async () => {
vi.useRealTimers();
const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY);
-
- element.date = '2023-07-27T12:00:00.000Z';
- element.requestUpdate();
- await elementIsStable(element);
-
- const time = element.shadowRoot!.querySelector('time');
- expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
-
- now.mockRestore();
+ try {
+ element.date = '2023-07-27T12:00:00.000Z';
+ element.requestUpdate();
+ await elementIsStable(element);
+
+ const time = element.shadowRoot!.querySelector('time');
+ expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z');
+ } finally {
+ now.mockRestore();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should fall back to raw string for relative values outside Intl range', async () => { | |
| vi.useRealTimers(); | |
| const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); | |
| element.date = '2023-07-27T12:00:00.000Z'; | |
| element.requestUpdate(); | |
| await elementIsStable(element); | |
| const time = element.shadowRoot!.querySelector('time'); | |
| expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); | |
| now.mockRestore(); | |
| }); | |
| it('should fall back to raw string for relative values outside Intl range', async () => { | |
| vi.useRealTimers(); | |
| const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); | |
| try { | |
| element.date = '2023-07-27T12:00:00.000Z'; | |
| element.requestUpdate(); | |
| await elementIsStable(element); | |
| const time = element.shadowRoot!.querySelector('time'); | |
| expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); | |
| } finally { | |
| now.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/format-relative-time/format-relative-time.test.ts` around
lines 412 - 424, The Date.now spy named now in the test 'should fall back to raw
string for relative values outside Intl range' can leak if an assertion throws;
wrap the setup and assertions so the spy is always restored (e.g., move
now.mockRestore() into a finally block or use a try/finally around the test body
or an afterEach that restores the spy), ensuring the spy created by
vi.spyOn(Date, 'now') is always cleaned up even if element.requestUpdate() or
the expect throws; reference the test name and the now spy to locate and update
the code.
Comment on lines
+283
to
+286
| const requestUpdate = vi.spyOn(element, 'requestUpdate'); | ||
| option.selected = true; | ||
|
|
||
| expect(requestUpdate).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Wait for observer delivery before asserting no requestUpdate calls
MutationObserver notifications are async, so asserting immediately after option.selected = true can miss a late callback and produce a false pass. Add a microtask/frame wait before the negative assertion.
Proposed fix
const requestUpdate = vi.spyOn(element, 'requestUpdate');
option.selected = true;
+ await new Promise(resolve => setTimeout(resolve, 0));
expect(requestUpdate).not.toHaveBeenCalled();As per coding guidelines, "**/*.test.ts: Follow unit testing patterns from .../testing-unit.md including createFixture and elementIsStable patterns."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const requestUpdate = vi.spyOn(element, 'requestUpdate'); | |
| option.selected = true; | |
| expect(requestUpdate).not.toHaveBeenCalled(); | |
| const requestUpdate = vi.spyOn(element, 'requestUpdate'); | |
| option.selected = true; | |
| await new Promise(resolve => setTimeout(resolve, 0)); | |
| expect(requestUpdate).not.toHaveBeenCalled(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/select/select.test.ts` around lines 283 - 286, The test
sets option.selected = true and immediately asserts requestUpdate wasn't called,
but MutationObserver callbacks are async; update the test to wait for observer
delivery before the negative assertion by using the project's test helper
pattern (e.g., call elementIsStable(element) or await the fixture/frame
microtask after setting option.selected) so any delayed requestUpdate calls run
first; locate the assertion around the spy on element.requestUpdate in
select.test.ts and insert the await/elementIsStable wait before the
expect(requestUpdate).not.toHaveBeenCalled() check.
Source: Coding guidelines
coryrylan
force-pushed
the
topic-cleanup-core
branch
from
84daad7 to
42af5d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
projects/core/src/format-relative-time/format-relative-time.test.ts (1)
412-424:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure
Date.nowspy is always restored, even on assertion failure.The spy cleanup on line 423 only runs if all assertions pass. If
expectthrows, the mock will leak into later tests.🛡️ Wrap cleanup in finally block
it('should fall back to raw string for relative values outside Intl range', async () => { vi.useRealTimers(); const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); - - element.date = '2023-07-27T12:00:00.000Z'; - element.requestUpdate(); - await elementIsStable(element); - - const time = element.shadowRoot!.querySelector('time'); - expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); - - now.mockRestore(); + try { + element.date = '2023-07-27T12:00:00.000Z'; + element.requestUpdate(); + await elementIsStable(element); + + const time = element.shadowRoot!.querySelector('time'); + expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); + } finally { + now.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/format-relative-time/format-relative-time.test.ts` around lines 412 - 424, The Date.now spy (variable now) must be restored in a finally block to avoid leaking the mock if expect throws; update the test case (the it('should fall back to raw string for relative values outside Intl range', ...) block) to wrap the setup, DOM checks and assertions inside a try { ... } finally { now.mockRestore(); vi.useRealTimers(); } (or at minimum call now.mockRestore() in finally) so the spy is always restored even on assertion failure; refer to Date.now spy variable name now, and the calls element.requestUpdate(), await elementIsStable(element), and the time query selector to locate the code to wrap.projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
13-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure fixture teardown always runs.
removeFixture(fixture)is only reached on the success path. IfelementIsStableorrunAxethrows, the fixture remains indocument.body, which can leak DOM across tests and cause follow-on flakiness.🛡️ Proposed fix to guarantee cleanup
describe(PagePanel.metadata.tag, () => { it('should pass axe check', async () => { const fixture = await createFixture(html` <nve-page> <nve-page-panel id="panel"> <nve-page-panel-header>panel heading</nve-page-panel-header> <nve-icon-button slot="actions" commandfor="panel" command="--close" icon-name="cancel" aria-label="close panel"> </nve-icon-button> <nve-page-panel-content>panel content</nve-page-panel-content> <nve-page-panel-footer>panel footer</nve-page-panel-footer> </nve-page-panel> </nve-page> `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 - 35, The test currently calls removeFixture(fixture) only on the success path which can leak DOM if elementIsStable or runAxe throws; wrap the test body that uses createFixture, elementIsStable, runAxe and PagePanel.metadata.tag in a try/finally and call removeFixture(fixture) from the finally block (ensuring fixture is declared in the outer scope so finally can access it) so cleanup always runs regardless of failures.projects/core/src/forms/control/control.css (1)
188-191:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stylelint empty-line violation before
width.Line 190 violates
declaration-empty-line-before; an empty line is required before thewidthdeclaration.🛠️ Suggested fix
:host([nve-control='inline']) { --control-height: auto; + width: fit-content;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/forms/control/control.css` around lines 188 - 191, The CSS block for :host([nve-control='inline']) is missing the required empty line before the width declaration; update the declaration block for the selector :host([nve-control='inline']) so there is a blank line immediately before the width property (i.e., ensure an empty line separates --control-height and width) to satisfy the declaration-empty-line-before rule.projects/core/src/select/select.test.ts (1)
270-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWait for observer delivery before asserting no
requestUpdatecalls.
MutationObservercallbacks are async, so asserting immediately afteroption.selected = truecan miss a late callback and produce a false pass. Add a microtask wait before the negative assertion to ensure any pending observer callbacks have fired.🐛 Proposed fix
const requestUpdate = vi.spyOn(element, 'requestUpdate'); option.selected = true; + await elementIsStable(element); expect(requestUpdate).not.toHaveBeenCalled();As per coding guidelines, "
**/*.test.ts: Follow unit testing patterns from .../testing-unit.md including createFixture and elementIsStable patterns."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/select/select.test.ts` around lines 270 - 287, The negative assertion runs before the MutationObserver could fire; after setting option.selected = true in the 'should stop observing removed option selected state changes' test, wait for observer delivery (e.g. await a microtask or call elementIsStable(element) again) before asserting that element.requestUpdate was not called so any pending observer callbacks can run; update the test around the option.selected assignment and expect(requestUpdate).not.toHaveBeenCalled() to include that microtask wait using the existing elementIsStable helper.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/internal/controllers/type-native-popover.controller.ts`:
- Around line 73-81: hostConnected() can schedule multiple async continuations
that each call `#setupLegacyTriggers`() and push duplicate observers into
`#observers`; fix by adding a per-instance cancellation token or incrementing
runId (e.g., this.#setupRunId) at the start of hostConnected() and capturing it
in the async continuation/await chain inside hostConnected() and inside
`#setupLegacyTriggers`(), then bail out if the captured id no longer matches the
current token before mutating `#observers` or installing listeners; apply the same
guard to the other async setup block referenced around lines 165-179 so stale
continuations cannot add observers after a disconnect/reconnect cycle.
- Around line 196-233: When switching an existing legacy trigger to 'hint' mode
the code never clears its prior native wiring because `#previousLegacyTrigger` ===
trigger prevents `#clearLegacyTrigger` from running; update `#updateLegacyTriggers`
to explicitly clear native wiring for the same trigger before installing hint
handlers: detect when host.popoverType === 'hint' and the current trigger
already has native attributes/fields (e.g., popovertarget attribute or
popoverTargetElement) or when `#previousLegacyTrigger` === trigger, call
this.#clearLegacyTrigger(trigger) (or otherwise remove
popovertarget/popoverTargetElement) and this.#removeHintTrigger() as needed,
then proceed to call this.#setupHintTrigger(trigger) and set
`#previousLegacyTrigger` = trigger so the button no longer retains native click
behavior.
- Around line 103-113: hostDisconnected() currently removes event listeners but
leaves a possible legacy trigger referencing the detached host; update
hostDisconnected to check this.#previousLegacyTrigger and, if present, clear its
popoverTargetElement property (set to null), remove the popovertarget attribute
(removeAttribute('popovertarget')), and then set this.#previousLegacyTrigger =
undefined (or null) so the trigger no longer holds a strong reference to the
host; also ensure this cleanup complements the existing
this.#removeHintTrigger() call.
In `@projects/core/src/internal/utils/dom.ts`:
- Around line 273-278: The catch assumes the thrown value is an Error and uses
(e as Error).message which can be undefined for non-Error throws; update the
EyeDropperConstructor try/catch so the LogService.warn logs a defensive error
message: detect if e is an Error and use e.message, else if it's a string use it
directly, otherwise attempt JSON.stringify(e) and fall back to String(e) on
failure, then include that derived message in the `EyeDropper selection failed:`
log; keep the existing return '' behavior.
- Around line 263-279: Add a JSDoc block above the exported function
openEyeDropper describing its purpose (open the browser EyeDropper and return
the selected color), its return type (Promise<string> — sRGB hex color or empty
string if unavailable/failed), behavior (logs a warning and returns '' when the
API is unavailable or selection fails), and any side effects (uses
LogService.warn). Follow the Google JSDoc style: include a one-line summary, a
longer description if needed, an `@returns` tag with the Promise<string>
explanation, and note that there are no parameters; place the comment
immediately above the openEyeDropper declaration.
In `@projects/core/src/select/select.ts`:
- Around line 184-202: The cleanup in `#syncOptionSelectedStates` currently
filters this.#observers inside the forEach for each removed option, causing
repeated O(n×m) passes; instead, collect removed observers in a temporary Set
(from this.#optionObservers entries where option is not in the new
this.#options) and perform a single this.#observers = this.#observers.filter(o
=> !removedSet.has(o)) pass, while still disconnecting and deleting each removed
observer from this.#optionObservers; update the logic inside
`#syncOptionSelectedStates` (references: this.#options, this.#optionObservers,
this.#observers, getElementUpdate, requestUpdate) to batch removals into one
filter operation.
---
Duplicate comments:
In `@projects/core/src/format-relative-time/format-relative-time.test.ts`:
- Around line 412-424: The Date.now spy (variable now) must be restored in a
finally block to avoid leaking the mock if expect throws; update the test case
(the it('should fall back to raw string for relative values outside Intl range',
...) block) to wrap the setup, DOM checks and assertions inside a try { ... }
finally { now.mockRestore(); vi.useRealTimers(); } (or at minimum call
now.mockRestore() in finally) so the spy is always restored even on assertion
failure; refer to Date.now spy variable name now, and the calls
element.requestUpdate(), await elementIsStable(element), and the time query
selector to locate the code to wrap.
In `@projects/core/src/forms/control/control.css`:
- Around line 188-191: The CSS block for :host([nve-control='inline']) is
missing the required empty line before the width declaration; update the
declaration block for the selector :host([nve-control='inline']) so there is a
blank line immediately before the width property (i.e., ensure an empty line
separates --control-height and width) to satisfy the
declaration-empty-line-before rule.
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-35: The test currently calls removeFixture(fixture) only on the
success path which can leak DOM if elementIsStable or runAxe throws; wrap the
test body that uses createFixture, elementIsStable, runAxe and
PagePanel.metadata.tag in a try/finally and call removeFixture(fixture) from the
finally block (ensuring fixture is declared in the outer scope so finally can
access it) so cleanup always runs regardless of failures.
In `@projects/core/src/select/select.test.ts`:
- Around line 270-287: The negative assertion runs before the MutationObserver
could fire; after setting option.selected = true in the 'should stop observing
removed option selected state changes' test, wait for observer delivery (e.g.
await a microtask or call elementIsStable(element) again) before asserting that
element.requestUpdate was not called so any pending observer callbacks can run;
update the test around the option.selected assignment and
expect(requestUpdate).not.toHaveBeenCalled() to include that microtask wait
using the existing elementIsStable helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 278608d1-cca1-4b42-9a8b-2e34fa297ec4
📒 Files selected for processing (37)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.ts
Comment on lines
103
to
+113
| hostDisconnected() { | ||
| this.#observers.forEach(observer => observer.disconnect()); | ||
| this.#observers.length = 0; | ||
| this.host.removeEventListener('beforetoggle', this.#onBeforeToggle); | ||
| this.host.removeEventListener('toggle', this.#onToggle as EventListener); | ||
| this.host.removeEventListener('command', this.#onCommand as EventListener); | ||
| this.host.removeEventListener('interest', this.#onInterest as EventListener); | ||
| this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener); | ||
| this.host.removeEventListener('pointerdown', this.#onPointerDown); | ||
| this.host.removeEventListener('pointerup', this.#onPointerUp); | ||
| this.#removeHintTrigger(); |
There was a problem hiding this comment.
Disconnect cleanup still leaves the legacy trigger retaining the popover.
hostDisconnected() removes listeners, but it never clears #previousLegacyTrigger or the trigger’s popoverTargetElement/popovertarget. If the trigger stays in the DOM after the popover is removed, that button still holds a strong reference to the detached host and can keep routing native popover actions at a disconnected node.
Suggested fix
hostDisconnected() {
this.#observers.forEach(observer => observer.disconnect());
this.#observers.length = 0;
+ if (this.#previousLegacyTrigger) {
+ this.#clearLegacyTrigger(this.#previousLegacyTrigger);
+ this.#previousLegacyTrigger = null;
+ }
this.host.removeEventListener('beforetoggle', this.#onBeforeToggle);
this.host.removeEventListener('toggle', this.#onToggle as EventListener);
this.host.removeEventListener('command', this.#onCommand as EventListener);
this.host.removeEventListener('interest', this.#onInterest as EventListener);
this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hostDisconnected() { | |
| this.#observers.forEach(observer => observer.disconnect()); | |
| this.#observers.length = 0; | |
| this.host.removeEventListener('beforetoggle', this.#onBeforeToggle); | |
| this.host.removeEventListener('toggle', this.#onToggle as EventListener); | |
| this.host.removeEventListener('command', this.#onCommand as EventListener); | |
| this.host.removeEventListener('interest', this.#onInterest as EventListener); | |
| this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener); | |
| this.host.removeEventListener('pointerdown', this.#onPointerDown); | |
| this.host.removeEventListener('pointerup', this.#onPointerUp); | |
| this.#removeHintTrigger(); | |
| hostDisconnected() { | |
| this.#observers.forEach(observer => observer.disconnect()); | |
| this.#observers.length = 0; | |
| if (this.#previousLegacyTrigger) { | |
| this.#clearLegacyTrigger(this.#previousLegacyTrigger); | |
| this.#previousLegacyTrigger = null; | |
| } | |
| this.host.removeEventListener('beforetoggle', this.#onBeforeToggle); | |
| this.host.removeEventListener('toggle', this.#onToggle as EventListener); | |
| this.host.removeEventListener('command', this.#onCommand as EventListener); | |
| this.host.removeEventListener('interest', this.#onInterest as EventListener); | |
| this.host.removeEventListener('loseinterest', this.#onLoseInterest as EventListener); | |
| this.host.removeEventListener('pointerdown', this.#onPointerDown); | |
| this.host.removeEventListener('pointerup', this.#onPointerUp); | |
| this.#removeHintTrigger(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/internal/controllers/type-native-popover.controller.ts`
around lines 103 - 113, hostDisconnected() currently removes event listeners but
leaves a possible legacy trigger referencing the detached host; update
hostDisconnected to check this.#previousLegacyTrigger and, if present, clear its
popoverTargetElement property (set to null), remove the popovertarget attribute
(removeAttribute('popovertarget')), and then set this.#previousLegacyTrigger =
undefined (or null) so the trigger no longer holds a strong reference to the
host; also ensure this cleanup complements the existing
this.#removeHintTrigger() call.
johnyanarella
approved these changes
Jun 11, 2026
Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cleanup-core
branch
from
42af5d7 to
a84e891
Compare
github-actions
Bot
added
scope(ci)
dependencies
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
13-34:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFixture cleanup is not guaranteed if assertions fail.
The
removeFixture(fixture)call on line 34 is only reached on the success path. IfelementIsStableorrunAxethrows, the fixture remains indocument.body, causing DOM leakage and potential flakiness in subsequent tests. This issue was previously flagged and remains unresolved.🛡️ Proposed fix using try-finally
it('should pass axe check', async () => { const fixture = await createFixture(html` <nve-page> <nve-page-panel id="panel"> <nve-page-panel-header>panel heading</nve-page-panel-header> <nve-icon-button slot="actions" commandfor="panel" command="--close" icon-name="cancel" aria-label="close panel"> </nve-icon-button> <nve-page-panel-content>panel content</nve-page-panel-content> <nve-page-panel-footer>panel footer</nve-page-panel-footer> </nve-page-panel> </nve-page> `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 - 34, The removeFixture(fixture) call in the should pass axe check test is only executed on the success path, so if elementIsStable or runAxe throws an error, the fixture will leak into the DOM. Wrap the test logic (from creating the fixture through the assertions) in a try-finally block, moving the removeFixture(fixture) call into the finally block to guarantee cleanup regardless of whether the test passes or fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/select/select.ts`:
- Around line 206-215: The Select component has memory leaks because event
listeners registered in the `#setupCustomSelectUI`() method via onListboxActivate
are never tracked or cleaned up. Additionally, observers registered in
firstUpdated() are not re-registered when the component reattaches. Refactor
onListboxActivate to return a cleanup function, then track these cleanup
functions in the `#observers` array alongside the other observer cleanup handlers.
In disconnectedCallback(), ensure all cleanup functions from `#observers` are
called. In connectedCallback(), call `#setupCustomSelectUI`() (or re-register the
observers) to restore listeners when the component is reattached after being
disconnected.
---
Duplicate comments:
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-34: The removeFixture(fixture) call in the should pass axe
check test is only executed on the success path, so if elementIsStable or runAxe
throws an error, the fixture will leak into the DOM. Wrap the test logic (from
creating the fixture through the assertions) in a try-finally block, moving the
removeFixture(fixture) call into the finally block to guarantee cleanup
regardless of whether the test passes or fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 85f3e2ba-f130-4b89-b5f6-269f35f96b78
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
pnpm-workspace.yamlprojects/core/src/checkbox/checkbox.test.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.tsprojects/starters/angular/package.jsonprojects/starters/nuxt/package.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
13-34:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFixture cleanup is not guaranteed if assertions fail.
The
removeFixture(fixture)call on line 34 is only reached on the success path. IfelementIsStableorrunAxethrows, the fixture remains indocument.body, causing DOM leakage and potential flakiness in subsequent tests. This issue was previously flagged and remains unresolved.🛡️ Proposed fix using try-finally
it('should pass axe check', async () => { const fixture = await createFixture(html` <nve-page> <nve-page-panel id="panel"> <nve-page-panel-header>panel heading</nve-page-panel-header> <nve-icon-button slot="actions" commandfor="panel" command="--close" icon-name="cancel" aria-label="close panel"> </nve-icon-button> <nve-page-panel-content>panel content</nve-page-panel-content> <nve-page-panel-footer>panel footer</nve-page-panel-footer> </nve-page-panel> </nve-page> `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 - 34, The removeFixture(fixture) call in the should pass axe check test is only executed on the success path, so if elementIsStable or runAxe throws an error, the fixture will leak into the DOM. Wrap the test logic (from creating the fixture through the assertions) in a try-finally block, moving the removeFixture(fixture) call into the finally block to guarantee cleanup regardless of whether the test passes or fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/select/select.ts`:
- Around line 206-215: The Select component has memory leaks because event
listeners registered in the `#setupCustomSelectUI`() method via onListboxActivate
are never tracked or cleaned up. Additionally, observers registered in
firstUpdated() are not re-registered when the component reattaches. Refactor
onListboxActivate to return a cleanup function, then track these cleanup
functions in the `#observers` array alongside the other observer cleanup handlers.
In disconnectedCallback(), ensure all cleanup functions from `#observers` are
called. In connectedCallback(), call `#setupCustomSelectUI`() (or re-register the
observers) to restore listeners when the component is reattached after being
disconnected.
---
Duplicate comments:
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-34: The removeFixture(fixture) call in the should pass axe
check test is only executed on the success path, so if elementIsStable or runAxe
throws an error, the fixture will leak into the DOM. Wrap the test logic (from
creating the fixture through the assertions) in a try-finally block, moving the
removeFixture(fixture) call into the finally block to guarantee cleanup
regardless of whether the test passes or fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 85f3e2ba-f130-4b89-b5f6-269f35f96b78
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
pnpm-workspace.yamlprojects/core/src/checkbox/checkbox.test.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.tsprojects/starters/angular/package.jsonprojects/starters/nuxt/package.json
🛑 Comments failed to post (1)
projects/core/src/select/select.ts (1)
206-215:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify Select observer lifecycle registration points. # Expected: setup methods appear in firstUpdated, teardown in disconnectedCallback, # and no equivalent re-registration in connectedCallback. rg -n -C3 'firstUpdated|connectedCallback|disconnectedCallback|`#syncSelectValueStates`|`#setupOverflowListener`|`#syncOptionSelectedStates`|`#observers`' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 2358
🏁 Script executed:
#!/bin/bash # Check for the `#setupCustomSelectUI` method and any setup-related code rg -n '`#setupCustomSelectUI`' projects/core/src/select/select.ts -A 20 # Also search for test files that might show reattachment patterns fd -e spec.ts -e test.ts -e .test.ts 'select' --search-path projects/core/srcRepository: NVIDIA/elements
Length of output: 1736
🏁 Script executed:
#!/bin/bash # Check the select test file for any reattachment or lifecycle patterns head -100 projects/core/src/select/select.test.ts # Also check what onListboxActivate does - is it a persistent listener? rg -n 'onListboxActivate' projects/core/src -B2 -A5 | head -40 # Check if there are any other components with connectedCallback/disconnectedCallback patterns fd -e ts 'src/' --search-path projects/core | xargs rg -l 'connectedCallback|disconnectedCallback' | head -5Repository: NVIDIA/elements
Length of output: 6784
🏁 Script executed:
#!/bin/bash # Get the full implementation of onListboxActivate sed -n '77,100p' projects/core/src/internal/utils/focus.ts # Check if onListboxActivate returns a cleanup function or registers event listeners directly rg -n 'onListboxActivate' projects/core/src/internal/utils/focus.ts -A 15 # Check if there are any tests showing reattachment scenarios rg -n 'removeChild|remove\(\)|reattach|reconnect' projects/core/src/select/select.test.tsRepository: NVIDIA/elements
Length of output: 1590
🏁 Script executed:
#!/bin/bash # Get the complete onListboxActivate function sed -n '77,110p' projects/core/src/internal/utils/focus.ts # Check if disconnectCallback is tested anywhere rg -n 'disconnectCallback' projects/core/src/select/select.test.tsRepository: NVIDIA/elements
Length of output: 1021
🏁 Script executed:
#!/bin/bash # Check if there are any integration tests or examples showing component reattachment fd -e ts -e js 'select' projects/core | xargs rg -l 'appendChild|removeChild|remove\(\)' | head -5 # Look for patterns where elements might be reused/reattached in the test suite rg -n 'remove|detach' projects/core/src/select/select.test.ts -C 3 # Check if there are lifecycle tests in general for other components fd -e test.ts 'projects/core' | xargs rg -l 'disconnectCallback|connectedCallback' | head -3Repository: NVIDIA/elements
Length of output: 4354
🏁 Script executed:
#!/bin/bash # Check how removeFixture is implemented - does it truly remove and reattach? rg -n 'removeFixture' projects/core/@internals/testing -B 2 -A 10 | head -40 # Check if there are any Lit lifecycle patterns in similar components fd -type f -name '*.ts' projects/core/src | xargs rg -l 'connectedCallback|firstUpdated' | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && rg -n "connectedCallback|firstUpdated|disconnectedCallback" {} -A 3' # Specifically check for hasUpdated usage patterns rg -n 'hasUpdated' projects/core/src --type ts | head -10Repository: NVIDIA/elements
Length of output: 1239
🏁 Script executed:
#!/bin/bash # Check the complete onListboxActivate to see if it returns anything for cleanup cat projects/core/src/internal/utils/focus.ts | sed -n '77,120p' # Check what getElementUpdate and onChildListMutation return - are they observable cleanup mechanisms? rg -n 'export.*getElementUpdate|export.*onChildListMutation' projects/core/src --type ts -A 5 | head -40 # Look at the imports in select.ts to understand the utility functions head -30 projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 3166
🏁 Script executed:
#!/bin/bash # Get full implementations of getElementUpdate and onChildListMutation sed -n '107,130p' projects/core/src/internal/utils/dom.ts sed -n '9,30p' projects/core/src/internal/utils/events.tsRepository: NVIDIA/elements
Length of output: 1601
🏁 Script executed:
#!/bin/bash # Verify: Are event listeners from onListboxActivate ever cleaned up anywhere? rg -n 'removeEventListener|onListboxActivate' projects/core/src/select/select.ts # Check if there's a `#setupCustomSelectUI` cleanup method rg -n '`#setupCustomSelectUI`|`#cleanupCustomSelectUI`' projects/core/src/select/select.ts -A 5 # Confirm the full disconnectCallback implementation sed -n '211,220p' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 976
🏁 Script executed:
#!/bin/bash # Check if onListboxActivate return value is used anywhere rg -n 'onListboxActivate' projects/core/src/select/select.ts -B 1 -A 2 # Verify: does setupCustomSelectUI store anything returned from onListboxActivate? sed -n '234,245p' projects/core/src/select/select.ts # Check if there are any other cleanup mechanisms for setupCustomSelectUI rg -n '`#setupCustomSelectUI`|cleanup|dispose' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 877
Re-register Select observers and clean up event listeners on lifecycle transitions
Line 213 disconnects tracked observers (
#observersarray), but#setupCustomSelectUI()(line 234) registers event listeners viaonListboxActivatethat are never tracked or cleaned up, causing memory leaks on component detach. Additionally, when the component reattaches,connectedCallbackdoes not re-register the observers because setup only happens once infirstUpdated()(line 167).Suggested fix
`#setupCustomSelectUI`() { if (this.#select?.size === 0) { - onListboxActivate(this.#select, () => { + this.#customSelectUICleanup?.(); + const unsubscribe = onListboxActivate(this.#select, () => { this.requestUpdate(); // update menu items this.#dropdown!.style.setProperty('--min-width', `${this.#input!.getBoundingClientRect().width}px`); this.#dropdown!.showPopover(); focusElementTimeout(this.#menuItems[0] as HTMLElement); }); + this.#customSelectUICleanup = unsubscribe; } } connectedCallback() { super.connectedCallback(); appendRootNodeStyle(this, globalStyles); + if (this.hasUpdated && this.#observers.length === 0) { + this.#syncSelectValueStates(); + this.#setupOverflowListener(); + this.#syncOptionSelectedStates(); + this.#setupCustomSelectUI(); + } } disconnectedCallback() { super.disconnectedCallback(); this.#observers.forEach(observer => observer.disconnect()); + this.#observers.length = 0; this.#optionObservers.clear(); + this.#customSelectUICleanup?.(); }Note:
onListboxActivatemust be refactored to return a cleanup function for this pattern to work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/select/select.ts` around lines 206 - 215, The Select component has memory leaks because event listeners registered in the `#setupCustomSelectUI`() method via onListboxActivate are never tracked or cleaned up. Additionally, observers registered in firstUpdated() are not re-registered when the component reattaches. Refactor onListboxActivate to return a cleanup function, then track these cleanup functions in the `#observers` array alongside the other observer cleanup handlers. In disconnectedCallback(), ensure all cleanup functions from `#observers` are called. In connectedCallback(), call `#setupCustomSelectUI`() (or re-register the observers) to restore listeners when the component is reattached after being disconnected.
- Added tests to ensure that the checked state of checkboxes and radio buttons resets to their native defaults upon calling the reset method. - Improved the handling of observers in form controls to prevent duplication after reconnecting. - Enhanced the Select component to stop observing removed options and to reset selected options to their native defaults. - Added accessibility tests for the PagePanel component to ensure compliance with axe standards. Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-cleanup-core
branch
from
a84e891 to
18c8199
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
projects/core/src/page/page-panel/page-panel.test.axe.ts (1)
13-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnsure fixture teardown always runs.
removeFixture(fixture)is only reached on the success path. IfelementIsStableorrunAxethrows, the fixture remains indocument.body, which can leak DOM across tests and cause follow-on flakiness.🧹 Suggested fix
describe(PagePanel.metadata.tag, () => { it('should pass axe check', async () => { const fixture = await createFixture(html` <nve-page> <nve-page-panel id="panel"> <nve-page-panel-header>panel heading</nve-page-panel-header> <nve-icon-button slot="actions" commandfor="panel" command="--close" icon-name="cancel" aria-label="close panel"> </nve-icon-button> <nve-page-panel-content>panel content</nve-page-panel-content> <nve-page-panel-footer>panel footer</nve-page-panel-footer> </nve-page-panel> </nve-page> `); - await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); - const results = await runAxe([PagePanel.metadata.tag]); - expect(results.violations.length).toBe(0); - removeFixture(fixture); + try { + await elementIsStable(fixture.querySelector(PagePanel.metadata.tag)); + const results = await runAxe([PagePanel.metadata.tag]); + expect(results.violations.length).toBe(0); + } finally { + removeFixture(fixture); + } }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/page/page-panel/page-panel.test.axe.ts` around lines 13 - 35, The removeFixture call in the test is only executed on the success path, meaning if elementIsStable or runAxe throws an error, the fixture will remain in the DOM and leak across tests. Wrap the test logic in a try-finally block where the try block contains the createFixture call and all test assertions (elementIsStable, runAxe, and expect), and move the removeFixture call to the finally block to ensure it always runs regardless of whether the test passes or fails.projects/core/src/format-relative-time/format-relative-time.test.ts (1)
412-424:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap spy cleanup in try-finally to prevent test pollution.
The
Date.nowspy cleanup at line 423 only executes if assertions succeed. If any assertion throws, the spy persists and may affect subsequent tests.🛡️ Proposed fix
it('should fall back to raw string for relative values outside Intl range', async () => { vi.useRealTimers(); const now = vi.spyOn(Date, 'now').mockReturnValue(Number.NEGATIVE_INFINITY); - - element.date = '2023-07-27T12:00:00.000Z'; - element.requestUpdate(); - await elementIsStable(element); - - const time = element.shadowRoot!.querySelector('time'); - expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); - - now.mockRestore(); + try { + element.date = '2023-07-27T12:00:00.000Z'; + element.requestUpdate(); + await elementIsStable(element); + + const time = element.shadowRoot!.querySelector('time'); + expect(time!.textContent!.trim()).toBe('2023-07-27T12:00:00.000Z'); + } finally { + now.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/format-relative-time/format-relative-time.test.ts` around lines 412 - 424, The spy cleanup call to now.mockRestore() at the end of the test is not guaranteed to execute if any assertion throws an error, which can cause the Date.now spy to persist and pollute subsequent tests. Wrap the test logic between creating the spy and the mockRestore call in a try-finally block, moving the now.mockRestore() call into the finally block to ensure cleanup always executes regardless of whether the test assertions pass or fail.projects/core/src/internal/utils/dom.ts (1)
275-277:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle non-
Errorthrows in EyeDropper failure logging.
catch (e)can receive non-Errorvalues, so(e as Error).messagecan becomeundefinedand lose useful diagnostics.💡 Proposed fix
try { return (await new EyeDropperConstructor().open()).sRGBHex; } catch (e) { - LogService.warn(`EyeDropper selection failed: ${(e as Error).message}`); + const message = + e instanceof Error + ? e.message + : typeof e === 'string' + ? e + : (() => { + try { + return JSON.stringify(e); + } catch { + return String(e); + } + })(); + LogService.warn(`EyeDropper selection failed: ${message}`); return ''; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/internal/utils/dom.ts` around lines 275 - 277, The catch block handling EyeDropper selection failure assumes the caught exception is always an Error object with a message property, but JavaScript can throw any type of value. When a non-Error value is caught, accessing .message becomes undefined and loses diagnostic information. In the catch block around the LogService.warn call, replace the direct (e as Error).message access with a safe error handling approach that works for any thrown value type, such as checking if e is an Error instance first, or converting any value to a string representation to ensure the log always contains meaningful error information.projects/core/src/forms/control/control.css (1)
189-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix stylelint empty-line violation before
width.Line 190 violates
declaration-empty-line-before; this can fail CI.🛠️ Suggested fix
:host([nve-control='inline']) { --control-height: auto; + width: fit-content;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/core/src/forms/control/control.css` around lines 189 - 190, The `width` property declaration in the control.css file violates the stylelint `declaration-empty-line-before` rule. Add an empty line between the `--control-height: auto;` declaration and the `width: fit-content;` declaration to comply with the stylelint configuration and prevent CI failures.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/internal/services/log.service.ts`:
- Line 30: Add a test case to cover the SSR/non-DOM environment branch where
globalThis.document is undefined. In the test suite for log.service.ts, create a
test that stubs globalThis.document as undefined, then calls the warn and error
methods and verifies they complete without throwing an error. This will ensure
the optional chaining guard on the dispatchEvent call is properly exercised and
the code gracefully handles environments without a document object.
In `@projects/core/src/select/select.ts`:
- Around line 214-215: The disconnect or cleanup method currently clears the
`#optionObservers` collection but does not clear the `#observers` array, which
can leave stale observer references if the component instance persists. After
clearing `#optionObservers`, also add a line to clear the `#observers` array
using the same clearing mechanism to ensure complete cleanup of all observer
references.
- Around line 186-204: The observer setup in firstUpdated() runs only once
during initial component initialization, so when the component is disconnected
from and reconnected to the DOM, the observers remain permanently disconnected
and stop tracking option state changes. Move the calls to
`#syncSelectValueStates`() and `#syncOptionSelectedStates`() from firstUpdated() to
connectedCallback() (or implement reconnectedCallback() if available) so that
observers are re-initialized each time the component connects to the DOM,
ensuring mutation observers continue to work across disconnect/reconnect cycles
for dynamically added/removed options.
---
Duplicate comments:
In `@projects/core/src/format-relative-time/format-relative-time.test.ts`:
- Around line 412-424: The spy cleanup call to now.mockRestore() at the end of
the test is not guaranteed to execute if any assertion throws an error, which
can cause the Date.now spy to persist and pollute subsequent tests. Wrap the
test logic between creating the spy and the mockRestore call in a try-finally
block, moving the now.mockRestore() call into the finally block to ensure
cleanup always executes regardless of whether the test assertions pass or fail.
In `@projects/core/src/forms/control/control.css`:
- Around line 189-190: The `width` property declaration in the control.css file
violates the stylelint `declaration-empty-line-before` rule. Add an empty line
between the `--control-height: auto;` declaration and the `width: fit-content;`
declaration to comply with the stylelint configuration and prevent CI failures.
In `@projects/core/src/internal/utils/dom.ts`:
- Around line 275-277: The catch block handling EyeDropper selection failure
assumes the caught exception is always an Error object with a message property,
but JavaScript can throw any type of value. When a non-Error value is caught,
accessing .message becomes undefined and loses diagnostic information. In the
catch block around the LogService.warn call, replace the direct (e as
Error).message access with a safe error handling approach that works for any
thrown value type, such as checking if e is an Error instance first, or
converting any value to a string representation to ensure the log always
contains meaningful error information.
In `@projects/core/src/page/page-panel/page-panel.test.axe.ts`:
- Around line 13-35: The removeFixture call in the test is only executed on the
success path, meaning if elementIsStable or runAxe throws an error, the fixture
will remain in the DOM and leak across tests. Wrap the test logic in a
try-finally block where the try block contains the createFixture call and all
test assertions (elementIsStable, runAxe, and expect), and move the
removeFixture call to the finally block to ensure it always runs regardless of
whether the test passes or fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 63632c82-5e08-4834-a9a8-fccb1cc29cd3
📒 Files selected for processing (37)
projects/core/src/checkbox/checkbox.test.tsprojects/core/src/combobox/combobox.test.lighthouse.tsprojects/core/src/dialog/dialog.test.lighthouse.tsprojects/core/src/drawer/drawer.test.lighthouse.tsprojects/core/src/dropdown-group/dropdown-group.test.lighthouse.tsprojects/core/src/format-relative-time/format-relative-time.test.tsprojects/core/src/format-relative-time/format-relative-time.tsprojects/core/src/forms/control-group/control-group.cssprojects/core/src/forms/control-group/control-group.tsprojects/core/src/forms/control/control.cssprojects/core/src/forms/control/control.test.tsprojects/core/src/forms/control/control.tsprojects/core/src/forms/utils/states.tsprojects/core/src/index.test.lighthouse.tsprojects/core/src/internal/controllers/keynav-grid.controller.test.tsprojects/core/src/internal/controllers/keynav-grid.controller.tsprojects/core/src/internal/controllers/keynav-list.controller.test.tsprojects/core/src/internal/controllers/keynav-list.controller.tsprojects/core/src/internal/controllers/type-native-popover.controller.test.tsprojects/core/src/internal/controllers/type-native-popover.controller.tsprojects/core/src/internal/services/log.service.test.tsprojects/core/src/internal/services/log.service.tsprojects/core/src/internal/utils/dom.test.tsprojects/core/src/internal/utils/dom.tsprojects/core/src/menu/menu.test.lighthouse.tsprojects/core/src/notification/notification.test.lighthouse.tsprojects/core/src/page/page-panel/page-panel.test.axe.tsprojects/core/src/page/page-panel/page-panel.test.ssr.tsprojects/core/src/pagination/pagination.test.lighthouse.tsprojects/core/src/preferences-input/preferences-input.test.lighthouse.tsprojects/core/src/radio/radio.test.tsprojects/core/src/select/select.test.tsprojects/core/src/select/select.tsprojects/core/src/sparkline/sparkline.utils.test.tsprojects/core/src/sparkline/sparkline.utils.tsprojects/core/src/toast/toast.test.lighthouse.tsprojects/core/src/tree/tree.test.lighthouse.ts
|
|
||
| static #dispatch(type: 'warn' | 'error', value: string, ...args: unknown[]) { | ||
| globalThis.document.dispatchEvent(new CustomEvent('NVE_ELEMENTS_LOG', { detail: { type, value, args } })); | ||
| globalThis.document?.dispatchEvent(new CustomEvent('NVE_ELEMENTS_LOG', { detail: { type, value, args } })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the no-document branch.
The guard is correct, but this SSR/non-DOM path is not explicitly asserted in the shown tests; add one case that stubs document as undefined and verifies warn/error do not throw.
🧪 Suggested test shape
+it('should not throw when document is unavailable', () => {
+ vi.stubGlobal('document', undefined);
+ expect(() => LogService.warn('test warn')).not.toThrow();
+ expect(() => LogService.error('test error')).not.toThrow();
+});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/internal/services/log.service.ts` at line 30, Add a test
case to cover the SSR/non-DOM environment branch where globalThis.document is
undefined. In the test suite for log.service.ts, create a test that stubs
globalThis.document as undefined, then calls the warn and error methods and
verifies they complete without throwing an error. This will ensure the optional
chaining guard on the dispatchEvent call is properly exercised and the code
gracefully handles environments without a document object.
Comment on lines
186
to
204
| #syncOptionSelectedStates() { | ||
| const options = new Set(this.#options); | ||
|
|
||
| this.#optionObservers.forEach((observer, option) => { | ||
| if (!options.has(option)) { | ||
| observer.disconnect(); | ||
| this.#optionObservers.delete(option); | ||
| this.#observers = this.#observers.filter(o => o !== observer); | ||
| } | ||
| }); | ||
|
|
||
| this.#options.forEach(o => { | ||
| if (!this.#trackedOptions.has(o)) { | ||
| this.#trackedOptions.add(o); | ||
| this.#observers.push(getElementUpdate(o, 'selected', () => this.requestUpdate())); | ||
| if (!this.#optionObservers.has(o)) { | ||
| const observer = getElementUpdate(o, 'selected', () => this.requestUpdate()); | ||
| this.#optionObservers.set(o, observer); | ||
| this.#observers.push(observer); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Select reconnect test coverage
rg -n -C3 --type=ts 'disconnect.*connect|reconnect' projects/core/src/select/Repository: NVIDIA/elements
Length of output: 41
🏁 Script executed:
# Read the select.ts file to understand the full lifecycle implementation
# First, get the file size
wc -l projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 98
🏁 Script executed:
# Look for disconnectedCallback and firstUpdated implementations
rg -n -B2 -A10 'disconnectedCallback|firstUpdated' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 997
🏁 Script executed:
# Search for Select test files more broadly
find projects/core -type f -name '*select*.test.*' -o -name '*select*.spec.*'Repository: NVIDIA/elements
Length of output: 432
🏁 Script executed:
# Search for any lifecycle-related lifecycle tests
rg -l 'disconnectedCallback|firstUpdated|reconnect' projects/core --type tsRepository: NVIDIA/elements
Length of output: 1600
🏁 Script executed:
# Check the main Select test file for disconnect/reconnect scenarios
rg -n 'disconnected|reconnect|removeChild|remove\(' projects/core/src/select/select.test.ts | head -20Repository: NVIDIA/elements
Length of output: 108
🏁 Script executed:
# Check if there's a reconnectedCallback pattern in other components
rg -n 'reconnectedCallback' projects/core/src --type ts | head -10Repository: NVIDIA/elements
Length of output: 41
🏁 Script executed:
# Read more of select.ts to see the full observer setup context
sed -n '175,180p' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 395
🏁 Script executed:
# Check if Select implements connectedCallback
rg -n 'connectedCallback' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 191
🏁 Script executed:
# Look at the full lifecycle methods and any attribute/property change handling
sed -n '160,230p' projects/core/src/select/select.tsRepository: NVIDIA/elements
Length of output: 2462
🏁 Script executed:
# Check how other components handle disconnect/reconnect lifecycle (e.g., Dropdown, Combobox)
rg -B2 -A8 'disconnectedCallback|connectedCallback' projects/core/src/dropdown/dropdown.ts projects/core/src/combobox/combobox.ts 2>/dev/null | head -40Repository: NVIDIA/elements
Length of output: 1589
Re-initialize observers in connectedCallback() for disconnect/reconnect scenarios.
The observers setup in firstUpdated() are disconnected when the component is removed from the DOM but never re-initialized. Since firstUpdated() only runs once, after a disconnect/reconnect cycle:
- The mutation observers won't trigger option state syncing when options change
- Dynamically added/removed options won't have their selected state tracked
- All existing observers remain permanently disconnected
Combobox implements the correct pattern by setting up observers in connectedCallback() rather than firstUpdated(), ensuring they're re-initialized on reconnect. Apply the same approach to Select: move observer initialization from #syncSelectValueStates() and #syncOptionSelectedStates() calls in firstUpdated() to connectedCallback(), or implement a reconnectedCallback() hook to re-establish observers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/select/select.ts` around lines 186 - 204, The observer
setup in firstUpdated() runs only once during initial component initialization,
so when the component is disconnected from and reconnected to the DOM, the
observers remain permanently disconnected and stop tracking option state
changes. Move the calls to `#syncSelectValueStates`() and
`#syncOptionSelectedStates`() from firstUpdated() to connectedCallback() (or
implement reconnectedCallback() if available) so that observers are
re-initialized each time the component connects to the DOM, ensuring mutation
observers continue to work across disconnect/reconnect cycles for dynamically
added/removed options.
Comment on lines
+214
to
215
| this.#optionObservers.clear(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider clearing the #observers array for complete cleanup.
While clearing #optionObservers is correct, the #observers array is not cleared after disconnecting. This leaves references to disconnected observers if the component instance persists.
♻️ Suggested addition
disconnectedCallback() {
super.disconnectedCallback();
this.#observers.forEach(observer => observer.disconnect());
+ this.#observers = [];
this.#optionObservers.clear();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/select/select.ts` around lines 214 - 215, The disconnect or
cleanup method currently clears the `#optionObservers` collection but does not
clear the `#observers` array, which can leave stale observer references if the
component instance persists. After clearing `#optionObservers`, also add a line
to clear the `#observers` array using the same clearing mechanism to ensure
complete cleanup of all observer references.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
reset()behavior to reliably restore native defaults across checkbox, radio, select, and other resettable inputs.